Skip to content

Conversation

@apappascs
Copy link
Contributor

Removes the retryTemplate.execute wrapper from the chatCompletionStream calls in MiniMax, MistralAI, and ZhiPuAI chat models.

following up on this comment #1852 (comment) from @markpollack

Removes the `retryTemplate.execute` wrapper from the `chatCompletionStream` calls in MiniMax, MistralAI, and ZhiPuAI chat models.

Signed-off-by: Alexandros Pappas <[email protected]>
@sobychacko
Copy link
Contributor

@apappascs The PR build fails - can you please take a look? https://github.com/spring-projects/spring-ai/actions/runs/15634414758/job/44072094143

@apappascs
Copy link
Contributor Author

sobychacko

thank you @sobychacko I am working on that. need to disable a few tests.

@apappascs apappascs marked this pull request as ready for review June 15, 2025 19:47
@sobychacko
Copy link
Contributor

@apappascs Curious why you disabled those tests. Did your changes make those tests fail? In that case, we need to investigate why that is the case and address it accordingly. If we leave these tests disabled, we risk missing regressions that would normally be caught by these tests, including failures unrelated to the current changes.

@apappascs
Copy link
Contributor Author

@apappascs Curious why you disabled those tests. Did your changes make those tests fail? In that case, we need to investigate why that is the case and address it accordingly. If we leave these tests disabled, we risk missing regressions that would normally be caught by these tests, including failures unrelated to the current changes.

Very valid points @sobychacko . I sould have linked this issue
#1193
for clarity. The retry method used for blocking calls isn't working as expected for the streaming calls. The tests are related to the code changes I did and have been disabled or removed for the other models as well.

@markpollack markpollack added the bug Something isn't working label Sep 28, 2025
@markpollack
Copy link
Member

@chemicL thoughts?

@chemicL
Copy link
Member

chemicL commented Sep 29, 2025

@chemicL thoughts?

#1193 (comment)
I am not sure whether this PR tackles any real problem? The retry template should not have any effects on reactive chains, unless their implementations throw, which should be a programming error. So removing them should not have a significant effect, the bulk of the work is the strategy for retries - whether the framework implements it or it's the user burden.

@apappascs
Copy link
Contributor Author

Thank you for the input @chemicL . I think this alone, as you mentioned, does nt really do much. We discussed last year about removing the retry from streamable calls because it didn't work properly as we expected.

I followed the same pattern we have for the non streamable calls:

#1193 (comment)

And your comment added to the context:
#1193 (comment)

@markpollack @tzolov This is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants